fix(cli): distinguish a skipped browser check from a genuinely clean one - #3901
fix(cli): distinguish a skipped browser check from a genuinely clean one#3901miga-heygen wants to merge 1 commit into
Conversation
`hyperframes check`'s runtime/layout/motion/contrast JSON sections reported the exact same ok:true/zero-findings shape whether the browser session actually ran and found nothing, or never ran at all (a blocking lint error, a lint-step crash, or an exception thrown before the audit could start all took the same "empty result" fallback). Only the top-level ok field and an undocumented layout.duration === 0 tell separated the two cases apart. Add a skipped boolean to CheckBrowserResult, set at the only two places one is constructed (false in the real audit result, true in the empty-result fallback), and mirror it to a new top-level browserSkipped field on CheckReport. The human-readable report now prints a warning when the browser never ran; the JSON envelope carries the same signal for callers that only check individual sections. Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
somanshreddy
left a comment
somanshreddy
left a comment
There was a problem hiding this comment.
Independent review at 5e74e2cf (manual pass — Codex spend-capped this session, so direct source verification, not a codex run). Verdict: correct, well-designed, no blockers. One minor completeness note.
Verified at source — the design is the right one
skipped/browserSkippedare required, not optional (checkTypes.ts). That's what makes this airtight: the compiler forces everyCheckBrowserResultconstruction to setskipped, so "cover every site" is a type obligation rather than a reviewer's grep. (It's why the test helperreportWithFindingshad to addbrowserSkipped: falseto compile.)- Construction sites are complete.
skippedis assigned a literal in exactly two places —runAuditGrid→false(checkPipeline.ts:1123, a session genuinely ran) andemptyBrowserResult()→true(:1504). Everybrowservalue reachingbuildReportflows from one of those two, andbuildReportmirrors it verbatim (browserSkipped: browser.skipped,:1379). Traced all four paths:- lint blocking error →
emptyBrowserResult()(:1150) → skipped ✓ - lint step itself throws →
failureReport(:1140→:1530) → skipped ✓ - browser check throws → catch →
emptyBrowserResult()(:1170) → skipped ✓ - real audit →
runAuditGrid→ not skipped ✓
- lint blocking error →
- Warning wording is carefully scoped. The human-report line (
check.ts) names onlylayout/motion/contrastas empty placeholders and deliberately excludesruntime, because the throw/lint-crash triggers leave a real diagnostic finding there. That nuance is correct and matches the type doc-comment.
Non-blocking: one of the three skip triggers is untested
The tests pin the lint-block and browser-throw triggers (browserSkipped: true), plus the normal (false) and JSON-envelope cases. The third trigger — the linter itself crashing (lintProject throws → failureReport, :1140) — has no committed test asserting browserSkipped: true. It's structurally safe (shares emptyBrowserResult(); the required field can't be undefined), so this is genuine test-debt rather than a latent bug — a one-line test with a throwing lintProject dep would close the last of the three documented paths. (Same shape, lower stakes: the printed warning line in printHumanReport is also unasserted — cosmetic.)
I confirmed the assertions are load-bearing by hand: hardcoding browserSkipped: false in buildReport fails both the lint-block and throw tests, so they bind the field (not mock-echo). I did not execute the suite locally (repo not provisioned in this sandbox; per your notes check.test.ts runs 61/61 for you and the full-package failures are pre-existing/unrelated) — this rests on static trace + your revert run.
Minor
browserSkippedis whole-session granularity — a--no-contrastrun still reportsbrowserSkipped: falsewith an emptycontrastsection. That's correct (the browser did run, and the skip was user-initiated), just worth knowing the field answers "did the session run," not "was every audit exercised." No change needed.
Not approving here — posting as a comment (I hold review, not stamp authority on this channel trigger). Clean on my pass; the note is test debt, your call whether to close it in this PR.
2 participants
Problem
hyperframes check'sruntime/layout/motion/contrastsections in the JSON report claimok:true/zero findings identically whether the browser session actually ran and found nothing, or never ran at all. Three distinct situations skip the browser entirely and fall through to the same empty-result shape:In all three cases, the top-level
okfield is correctlyfalse, but a caller (human or agent) that only inspects the individual sections sees a false "all clear" — the only tell was an undocumentedlayout.duration === 0.Fix
Add a
skipped: booleanfield toCheckBrowserResult, set at the only two places one is ever constructed:falsein the real audit result (runAuditGrid),truein the empty-result fallback (emptyBrowserResult(), used by all three skip paths above). Mirror it to a new top-levelbrowserSkipped: booleanonCheckReport, set inbuildReportdirectly frombrowser.skipped.check --jsonoutput now includesbrowserSkipped— the explicit signal a caller needs.Runtimesection, so the warning doesn't claimRuntimeitself is empty — onlylayout/motion/contrastalways are).browserSkippedin the envelope shape and call out the false-clean risk explicitly.Tests
packages/cli/src/commands/check.test.ts:browserSkipped: trueand thatlayout/motion/contrastreport clean/empty despite never having run.browserSkipped: falseon a normal completed run.browserSkipped: truewhen the browser check throws before producing results, and thatruntime(unlike the other sections) carries the real diagnostic finding in that case.browserSkipped: false.Verified the fix is load-bearing via a real revert: stashed the four production-code files (kept the test changes), reran — the four new/updated assertions failed exactly as expected (
browserSkippedreadundefined); restored, reran — 61/61 green again.Verification
bunx tsc --noEmit -p packages/cli— cleanbunx oxlint/bunx oxfmt --checkon all touched files — cleanbunx vitest run packages/cli/src/commands/check.test.ts— 61/61 passedbunx vitest run packages/cli(full package) — pre-existing failures only, unrelated to this diff (real-Chromium launch tests, macOS-onlyvm_statparsing, host telemetry env leakage, PID/socket ownership checks) — confirmed identical on an unmodified checkout before making any change.